Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update convert.py #1026

Closed
wants to merge 1 commit into from
Closed

Conversation

bpatterson971
Copy link

fix quoting in windows

crudely fix quoting in windows
@sampsyo
Copy link
Member

sampsyo commented Oct 22, 2014

Thanks for the contribution! This is clearly a bug.

What about filenames that contain single quotes? (This is why we're bothering with a full escape of the string.)

Maybe we'd be better off not using shell quoting or string templating at all—just using those $source and $dest tokens as replaceable components after a shlex.split and then invoking command_output with shell=False.

@bpatterson971
Copy link
Author

Thanks for taking a look!

In Windows, filenames with single quotes would work as long as the double quotes are surrounding the entire thing. I was able to convert files containing a single quote using this code.

@redbmk
Copy link

redbmk commented Oct 22, 2014

Double quotes should work in *nix as well, right? What if you just used a regex to replace double quotes with \" and then wrap the whole thing in double quotes? I'm thinking that same code would work for both windows and *nix systems.

@redbmk
Copy link

redbmk commented Oct 22, 2014

Scratch that...using double quotes might cause variables to be interpreted in *nix systems without further escaping

@sampsyo
Copy link
Member

sampsyo commented Oct 22, 2014

Ah, perhaps I should have asked: what if the filename has a double quote in it? 😃

This is why shell escaping is a debacle best avoided if possible...

@mluds
Copy link
Contributor

mluds commented Dec 20, 2014

I got this to mostly work using subprocess.list2cmdline which escapes args on Windows. See here. I'm still having trouble with certain characters like - and ' though, but only in some cases, which is weird.

@sampsyo
Copy link
Member

sampsyo commented Dec 21, 2014

Weird! Thanks for looking into this.

I think the most straightforward (and most reliable) solution will be to pass the command to subprocess as a list, removing the need for any escaping/quoting on our part. This shouldn't be too hard:

  1. Use shlex.split to break the template into components.
  2. Loop over the components and format them to fill in the filenames.
  3. Pass the resulting list as arguments.

Simple! Interested in tackling this, @mluds?

@mluds
Copy link
Contributor

mluds commented Dec 21, 2014

The only problem is with the --pretend flag where we need to display the command being executed as a string. I made a minimal fix in #1158 and I'd be happy to investigate further if there's any issues.

@sampsyo
Copy link
Member

sampsyo commented Dec 21, 2014

I'm not too worried about the --pretend output being perfectly copy-and-paste-able (this is already not true of the mv commands we emit). If it's OK with you, I'm going to take your fix in #1157.

sampsyo added a commit that referenced this pull request Dec 21, 2014
sampsyo added a commit that referenced this pull request Dec 21, 2014
@sampsyo
Copy link
Member

sampsyo commented Dec 21, 2014

Merged! Can you check whether I messed anything up, @mluds, or if this now works on Windows?

@sampsyo sampsyo closed this Dec 21, 2014
@mluds
Copy link
Contributor

mluds commented Dec 21, 2014

Oops, forgot to mention it didn't pass the Travis CI build! I'll work on that now.

@mluds
Copy link
Contributor

mluds commented Dec 21, 2014

Never mind, it looks like you got to it and it's working. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants